Skip to content

[QNN EP] Address PR #602 review comments (N-1 vacuous tests, T-1 line drift) - #719

Draft
yuhuchua-qti wants to merge 2 commits into
mainfrom
dev/yuhuchua/fix-pr-602-review-comment
Draft

[QNN EP] Address PR #602 review comments (N-1 vacuous tests, T-1 line drift)#719
yuhuchua-qti wants to merge 2 commits into
mainfrom
dev/yuhuchua/fix-pr-602-review-comment

Conversation

@yuhuchua-qti

@yuhuchua-qti yuhuchua-qti commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to PR #602 review comments from @huaychou.

[N-1] Group 10 ValidateCompiledModelCompatibilityInfo tests were vacuous on x86_64

ValidateCompiledModelCompatibilityInfo in qnn_execution_provider.cc has:

#if !defined(__aarch64__) && !defined(_M_ARM64) && !defined(_M_ARM64EC)
  ORT_CXX_LOG(logger_, ORT_LOGGING_LEVEL_WARNING, "Skip compatibility validation on x86 platforms.");
  *model_compatibility = OrtCompiledModelCompatibility_EP_NOT_APPLICABLE;
  return nullptr;
#endif

Any non-empty info string trips that guard on x86_64, so the previous TooFewFields_NotApplicable and BadVersionFormat_NotApplicable tests were both hitting the same x86-skip branch (empirically verified via ExpectLogged(WARNING, "Skip compatibility validation on x86 platforms.")).

Fix:

  • Consolidated TooFewFields_NotApplicable + BadVersionFormat_NotApplicable -> single NonEmptyOnX86Host_LogsSkipAndReturnsNotApplicable.
  • Renamed EmptyString_NotApplicable -> EmptyString_LogsNoInfoAndReturnsNotApplicable.
  • Both tests now ExpectLogged the actual WARNING each branch emits.
  • Group 10 header now documents that the field-parsing branches downstream of the x86 guard are unreachable on this coverage host; extracting the parser to a platform-guard-free free function is a follow-up option if we want unit-tier parser coverage.

[T-1] Absolute line-number references in comments

Replaced absolute source line numbers (line 301 / 547 / 1084) in 5 test comments with function names, so they don't drift when the source file changes. Line 547 had already drifted to 523 and 1084 to 1240 in the current source, illustrating the issue.

Bonus: enable_htp_fp16_clamp_overflow warning branch

Reviewer also asked whether enable_htp_fp16_clamp_overflow needs a unit test. The option was introduced in #686 (merged 2 days before #602) but wasn't picked up by #602's ctor option tests. Added Ctor_HtpFp16ClampOverflowTrueOnUnsupportedSdk_LogsWarning covering the ctor warning branch at qnn_execution_provider.cc:922-926. The coverage build (QAIRT 2.48 = QNN API 2.37) does not define QNN_HTP_FP16_CLAMP_OVERFLOW_AVAILABLE, so the #ifndef branch is compiled in and reachable.

Integration smoke coverage already exists in conv_test.cc:1195 (added by #686).

Test plan

  • onnxruntime_provider_test --gtest_filter="QnnUnit_ExecutionProviderTest.*:QnnUnit_ExecutionProviderHtpTest.*" — 65/65 pass
  • lint_and_fix clean

Ref: #602, #686

yuhuchua-qti and others added 2 commits August 13, 2026 11:07
…-number drift)

Follow-up to PR #602.

[N-1] Group 10 ValidateCompiledModelCompatibilityInfo tests were vacuous on
x86_64. ValidateCompiledModelCompatibilityInfo has an `#if !defined(__aarch64__)`
guard that logs WARNING "Skip compatibility validation on x86 platforms." and
returns EP_NOT_APPLICABLE before any field parsing. On x86_64 (the only host
where QNN_EP_INTERNAL_SYMBOL_ACCESS is set), any non-empty input trips the
guard, so the previous `TooFewFields` and `BadVersionFormat` tests were both
just testing the same x86-skip branch under different names.

- Consolidated `TooFewFields` + `BadVersionFormat` into a single honestly-named
  `NonEmptyOnX86Host_LogsSkipAndReturnsNotApplicable`.
- Renamed `EmptyString_NotApplicable` -> `EmptyString_LogsNoInfoAndReturnsNotApplicable`.
- Both tests now assert the actual WARNING log via `ExpectLogged`, empirically
  verifying which branch fires (confirmed the x86-skip WARNING fires for the
  6-field input `1:1.0.0:2.1.0:3.0.0:73:0`).
- Group 10 header now documents that the field-parsing branches are unreachable
  on this coverage host.

[T-1] Replaced absolute source line numbers (301 / 547 / 1084) in test comments
with function names, so they don't drift when the source file changes. Line 547
had already drifted to 523 and 1084 to 1240 in the current source.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds Ctor_HtpFp16ClampOverflowTrueOnUnsupportedSdk_LogsWarning covering the
ctor warning branch at qnn_execution_provider.cc:922-926, which fires when
enable_htp_fp16_clamp_overflow=1 but QNN_HTP_FP16_CLAMP_OVERFLOW_AVAILABLE is
not defined (QNN API < 2.38 / QAIRT < 2.49). The coverage build uses
QAIRT 2.48 = QNN API 2.37, so the warning branch is compiled in.

Addresses reviewer question on PR #602: `enable_htp_fp16_clamp_overflow` was
introduced in #686 (2 days before #602 merged) and had integration smoke test
coverage in conv_test.cc but no unit-tier coverage of the ctor warning.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant